@qfox @kzc this should nail the problem we had with test/ufuzz.js, so the workaround on the fuzzer to avoid this bug should no longer be required:

$ cat test.js
var NaN = 2;
console.log(NaN, Infinity * 0);

$ bin/uglifyjs test.js -c -o min.js
$ cat min.js
var NaN=2;console.log(NaN,0/0);

$ node test.js
2 NaN

$ node min.js
2 NaN

$ cat test.js | node
NaN NaN

$ cat min.js | node
NaN NaN

Cool. Shouldn't undefined also get the same treatment? They are the magic trio after all.

@qfox undefined already does that

Should I add builtin global function names like parseInt to the list as well? There's a few of those, they may be affected similarly.

@qfox no harm in trying to fuzz those as well 👍

I'm not so sure this change should be enabled in compress by default. The rationale for outputting NaN instead of 0/0 was that NaN parsed quicker and produced quicker code for the same file size in big data sets.

In jsperf testing NaN vs. 0/0 is a wash for FireFox 52 and Safari 9.1, but strangely on Chrome 57 NaN processing is 10% quicker than 0/0:

https://jsperf.com/test-nan-vs-0-0

Likewise, Infinity is 9% faster than 1/0 in jsperf tests on Chrome 57:

https://jsperf.com/infinity-vs-1-0-v2-0

At least the "use asm"; directive still works with this PR:

$ echo '"use asm"; console.log(NaN, Infinity)' | bin/uglifyjs -c
"use asm";console.log(NaN,Infinity);

$ echo '"use nop"; console.log(NaN, Infinity)' | bin/uglifyjs -c
"use nop";console.log(0/0,1/0);

Edit: Infinity test corrected.

https://jsperf.com/test-infinity-vs-0-1

Shouldn't that be 1/0 instead of 0/1, or am I going mad? 😅

While I think the performance implication is important, in a sense it is a correctness issue as before this PR the uglified code could misbehave in certain scenario, e.g. when wrapped inside an IIFE:

test.js

!function() {
    var NaN = 2;
    console.log(NaN, Infinity * 0);
}();

$ cat test.js | node
2 NaN

$ bin/uglifyjs -V
uglify-js 2.8.18

$ bin/uglifyjs test.js -c collapse_vars=0,reduce_vars=0 | node
2 2

$ git checkout master
$ bin/uglifyjs test.js -c collapse_vars=0,reduce_vars=0 | node
2 NaN

Shouldn't that be 1/0 instead of 0/1, or am I going mad?

The test was indeed wrong, but accurately described.

Here's the correct test:

https://jsperf.com/infinity-vs-1-0-v2-0

1/0 is 9% slower than Infinity on Chrome 57.

before this PR the uglified code could misbehave in certain scenario

That's just a plain old bug. A different issue altogether.

That's just a plain old bug. A different issue altogether.

I'm confused - do you mean we shouldn't fix this, or have you got a fix that does not suffer the performance issue?

The parens bug was introduced when changing NaN and Infinity to 0/0 and 1/0 respectively.

I think NaN should remain NaN because it's faster in Chrome, and does not ever require parens.

It is my view that this transform should be not enabled by default and if you must add it, it should be behind a new flag.

I think NaN should remain NaN because it's faster in Chrome, and does not have the need for parens in all cases.

Pretty sure #1723 (comment) would fail on Chrome as well?

uglify-js 2.8.18 would give, for that test case:

$ bin/uglifyjs test.js -c collapse_vars=0,reduce_vars=0
!function(){var NaN=2;console.log(NaN,NaN)}();

Pretty sure #1723 (comment) would fail on Chrome as well?

That's a plain old bug. A separate issue.

That's a plain old bug. A separate issue.

I must be pretty thick right now - what do you suggest as the correct course of action?

What I meant to say is, given the choice of:

• with this PR
  • bug is fixed on all engines
  • performance penalty is incurred on one engine
• without this PR
  • bug is present on all engines

I'd prefer the first one. Obviously if there is a better fix for the same bug that does not incur performance penalty on Chrome, I'm all for it.

I just can't think of one right now. 😓

Shall I open an issue with this performance penalty thing so we can keep track and fix it properly?

@alexlamsl Obviously we all want the bug fixed on all engines. Thank you for fixing it.

I am just arguing that NaN is better than 0/0 in the default case for reasons outlined above. A new ticket should be opened for that. Using NaN does not prevent program correctness in general. It's just the present implementation where it's an issue.

@kzc thanks for the clarification - I think we are on the same page now 👍

I've opened #1730 to track this.

Spec compliance aside, programs that redefine or override undefined, NaN and Infinity really should have no expectation of running. :-)

@kzc #1662 (comment)

When you think about it, the very act of transforming code in intricate and convoluted ways just to have a smaller version of itself that behaves the same at runtime has got to be on the top ten list of inane pursuits.

If this is your attempt to sum up my life, I think you're getting pretty close... 😈

When you think about it, the very act of transforming code in intricate and convoluted ways just to have a smaller version of itself that behaves the same at runtime has got to be on the top ten list of inane pursuits.

It's my bread and butter and I love it :D They're just giant puzzles, really.

They're just giant puzzles, really.

That's it exactly. Download bandwidth is a secondary concern.

Download bandwidth is a secondary concern.

Those people are missing out. It's like js1k; looking at a demo in no way describes the enjoyment of creating one.